Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean Code - Remove V1 implementation #1355

Merged
merged 1 commit into from
May 1, 2020

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Feb 5, 2020

Description

  • Remove the V1 code which should no longer more be used
  • Remove the V1 tests and testdata
  • Remove deprecated flags valid just for V1

Motivation

  • "The v1 projects are deprecated for too long and not supported since Feb 1, 2020.

Note that by removing the V1 we can:

  • Reduce the complexity of the code and tests
  • Reduce the time to check the result of tests
  • Remove the vendor's/deps and needs for its combability.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 5, 2020
@camilamacedo86 camilamacedo86 force-pushed the removeV1code branch 3 times, most recently from 6fafcb9 to b3c3950 Compare February 5, 2020 20:00
@camilamacedo86
Copy link
Member Author

/assing @droot

@Adirio
Copy link
Contributor

Adirio commented Feb 5, 2020

I said this in several places but will say it here as its where it makes sense.

I think we should not remove v1

The code has received a huge refactor lately and v1-v2 separation is quite clear right now. Other commits such as #1348 or #1290 will help in this sense even more. I would just leave v1 as it is right now, change the deprecation warning to something like: "v1 has been completely deprecated since 1st of February, 2020, and will no longer receive any fix or update. Upgrading your projects to v2 is highly recommended. In the following link you can find a step-by-step guide on how to do it: URL". I would probably also prevent the initialization of new v1 projects.

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Feb 5, 2020

Hi @Adirio,

Thank you for your input. Following my comments inline.

I said this in several places but will say it here as its where it makes sense.
The code has received a huge refactor lately and v1-v2 separation is quite clear right now.

Your work was not envious. It was great and indeed allow us to continue the lifecycle doing more cleanups. And then, the work made in the V2 will still as the code that will allow us to create the V3 in the future, however, the V1 was already flagged for no longer be supported and as I said many times we should not change/update and/or care so much because we knew that at some point it would be removed.

Why should we keep a code, tests, mocks, scripts, vendor and etc if they should no longer be used/updated/changed?

Note that by removing the V1 we can:

  • Reduce the complexity of the code and tests
  • Reduce the time to check the result of tests
  • Remove the vendor's/deps and needs for its combability.

So, IMO by removing all unnecessary V1 implementation it can simplify a lot. However, in POV we can keep the updated command to allow users are able to upgrade their projects to V2

@droot @mengqiy @DirectXMan12 WDYT?
PS.: PR is in WIP yet. ( I will probably wait for the plugin pr be merged o finish and to ask review on it)

@Adirio
Copy link
Contributor

Adirio commented Feb 6, 2020

I said this in several places but will say it here as its where it makes sense.
The code has received a huge refactor lately and v1-v2 separation is quite clear right now.

Your work was not envious. It was great and indeed allow us to continue the lifecycle doing more cleanups. And then, the work made in the V2 will still as the code that will allow us to create the V3 in the future, however, the V1 was already flagged for no longer be supported and as I said many times we should not change/update and/or care so much because we knew that at some point it would be removed.

I'm not worried about my code dissapearing. 😄 The point I was trying to make is that the situation when it was decided that v1 should be deprecated (3 months ago) and now is quite different, so despite being deprecated we may not need to remove the code completely.

Why should we keep a code, tests, mocks, scripts, vendor and etc if they should no longer be used/updated/changed?

Note that by removing the V1 we can:

  • Reduce the complexity of the code and tests
  • Reduce the time to check the result of tests
  • Remove the vendor's/deps and needs for its combability.

I do agree that we should remove the v1 tests from golden_test.sh, test.sh, and test_e2e.sh and thus the testdata/gopath dir. But I think that that is enough. I would keep the code for v1. Right now I would not say that v1 makes the code more complex outside of the mentioned tests.

Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 22 files deleted from testdata/package-v2* and both go.mod files which have three less lines. I think these removals are due to the make all test removal I mention in one of my comments. Please readd that line and run generate_golden.sh again to see if they get removed in that case.

.travis.yml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/internal/config.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/webhook.go Outdated Show resolved Hide resolved
generate_golden.sh Outdated Show resolved Hide resolved
test.sh Show resolved Hide resolved
Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot about this two lines

test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2020
Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are using this PR (removing v1) to introduce another completly different change (removing force flag and enforcing the resource to be scaffolded if it doesn't exist. This needs to be done in another PR not here.

Aside from that, a few NITs.

cmd/internal/config.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
generate_golden.sh Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 force-pushed the removeV1code branch 2 times, most recently from cba7f97 to 17578da Compare February 15, 2020 14:07
@camilamacedo86 camilamacedo86 changed the title WIP - Clean Code - Remove V1 Clean Code - Remove V1 testdata and code impl Feb 15, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 15, 2020
@camilamacedo86

This comment has been minimized.

@k8s-ci-robot
Copy link
Contributor

@camilamacedo86: GitHub didn't allow me to assign the following users: diroot.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @DirectXMan12
/assign @diroot
/assign @mengqiy

WDYT?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@camilamacedo86 camilamacedo86 changed the title Clean Code - Remove V1 testdata and code impl Clean Code - Remove V1 implementation Feb 15, 2020
@camilamacedo86 camilamacedo86 force-pushed the removeV1code branch 4 times, most recently from 8a7fb2e to f2fe2b1 Compare April 29, 2020 10:22
internal/config/config.go Outdated Show resolved Hide resolved
pkg/scaffold/api.go Outdated Show resolved Hide resolved
pkg/scaffold/init.go Outdated Show resolved Hide resolved
@@ -20,7 +20,7 @@ import (
"sigs.k8s.io/kubebuilder/pkg/model"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can fully remove the update command right @mengqiy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least remove it's logic and check updateCmd.HasSubCommands() before adding it, like kubebuilder does with alpha.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this file is no longer needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but IHMO we should remove the update command in a follow-up PR.
Then, if we need for any reason to do a revert will be easier.
The scope here is NOT to remove commands or deprecated args. The goal here is just clean up and remove the v1 code source.

test/e2e/v2/e2e_suite.go Outdated Show resolved Hide resolved
test/e2e/v3/e2e_suite.go Outdated Show resolved Hide resolved
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in the update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@camilamacedo86 camilamacedo86 Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we could/should remove. After thinking about and checking it I just think that it should be better done in a follow-up PR. note that we need to check/change the docs and all places that are speaking about it. So, shows out of the scope. WDYT? Could we agree in to do it a follow-up PR?

@camilamacedo86 camilamacedo86 force-pushed the removeV1code branch 2 times, most recently from 6e8987d to 0115566 Compare April 29, 2020 23:23
@camilamacedo86 camilamacedo86 force-pushed the removeV1code branch 2 times, most recently from d17db6b to 51b539c Compare April 30, 2020 01:11
@@ -67,7 +67,7 @@ var _ = Describe("kubebuilder", func() {
err := kbc.Init(
"--project-version", "3-alpha",
"--domain", kbc.Domain,
"--dep=false")
"--fetch-deps=false")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may miss something, but why do we switch to use the new flag? What's the motiviation?

Copy link
Member Author

@camilamacedo86 camilamacedo86 Apr 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some point, we received a suggestion to remove the deprecated flags. However, I revert this suggestion. It is not part of the scope of this PR. Here we are only removing the V1 code. So, good catcher. It is using dep again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is —dep not deprecated? It says so in code comments. Was this depreciation unrelated to the v2 change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is deprecated but still valid for v2.
We can remove the flag for V3 since is deprecated and I think we should do that.
However, it shows not part of the scope of this one and then, I think that would be better we have a specific or for that.

@camilamacedo86 camilamacedo86 force-pushed the removeV1code branch 2 times, most recently from f5855de to 76c13b3 Compare April 30, 2020 09:07
pkg/cli/cli.go Outdated Show resolved Hide resolved
@@ -65,7 +65,6 @@ var _ = Describe("kubebuilder", func() {
var controllerPodName string
By("init v2 project")
err := kbc.Init(
"--project-version", "2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still necessary now that the default project version is 3-alpha.

Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits then lgtm

@@ -0,0 +1,12 @@
package config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed.

Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 1bc124a into kubernetes-sigs:master May 1, 2020
@camilamacedo86 camilamacedo86 deleted the removeV1code branch May 1, 2020 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants